Skip to content

majit: retarget bare assertion-raise blocks instead of widening the pass - #1208

Merged
youknowone merged 1 commit into
mainfrom
residual
Aug 14, 2026
Merged

majit: retarget bare assertion-raise blocks instead of widening the pass#1208
youknowone merged 1 commit into
mainfrom
residual

Conversation

@youknowone

@youknowone youknowone commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Follow-up to #1206, addressing the Codex parity review's §1 finding on that PR and the P1 on the first revision of this one.

The finding

#1206 taught remove_assertion_errors to follow a one-block indirection. Codex objected twice, and both times correctly:

"accept a single-entry intermediate block" is broader than PyPy's direct-only condition (exit.target is graph.exceptblock). It can remove a conditional edge whose target block contains operations before raising AssertionError. join_blocks cannot justify this: it requires the predecessor to be unconditional with one exit (simplify.py:283-286), which a branch block is not.

Collapse or retarget the raise block in the corresponding simplification phase, then keep this pass's predicate literal, rather than retaining a second implementation with different eligibility rules.

The fix

remove_assertion_errors goes back to upstream's literal predicate. A new retarget_assert_raise_blocks runs immediately before it in simplify_lowered_graph and points an edge whose target exists only to raise the implicit AssertionError straight at exceptblock — where the flow space puts it upstream (flowcontext.py _implicit_), which is why the upstream predicate can be literal in the first place. The bypassed block keeps its exits and falls out as unreachable, the way eliminate_empty_blocks (simplify.py:52-69) leaves the blocks it rewires past.

A block qualifies when it has a single exit carrying that raise and every operation it holds satisfies can_remove_opCanRemove (simplify.py:411-423), the predicate the dead-op pass would apply to those operations once the raise is gone. can_remove_op rather than is_pure_op: the question is what may be deleted, and inline.rs:1130-1135 already records that upstream keeps LLOp.is_pure() and CanRemove distinct for that reason.

Two other placements were tried and rejected by measurement

  • Require the intermediate block to be empty. Disarms the change entirely — the raise blocks in this corpus build the exception. Back to three unrepairable constants_r entries and the in-process determinism failure.
  • Widen the front's panic_block_is_pure_message so collapse_panic_message_chains normalises these raises — the phase whose own doc says it collapses panic chains "to the bare raise so remove_assertion_errors prunes the branch as it does for a direct implicit raise". Also disarms the change. Instrumenting the rejection path showed why: that phase runs after simplify_lowered_graph, and neither do_warn_explicit nor w_member_get_direct_kind ever presents an assert-raising block to it. Its 24 rejections across the corpus are all genuine side-effecting calls (handle_alloc_error, os::exit, core::intrinsics::unreachable), so widening its predicate buys nothing here.

The normalisation therefore belongs in the pass sequence that runs before remove_assertion_errors, which is where it now sits.

Generated code is unchanged

Against #1206's guard at the same base and corpus (af392b9a):

#1206 guard this
all_jitcodes 2645 2645
jitcodes_index.bin identical
jitcode name list identical
jitcodes differing in bytecode, register counts, constant-pool sizes, result types, start points 0
unrepairable constants_r 1 (_unpackiterable_unknown_length 0x8) 1 (same)
in-process determinism 13/13 identical 13/13 identical

jitcodes.bin and descrs.bin are in HOST_ADDRESSED_OUTPUTS and are not comparable across processes, so the comparison is on the address-free fields.

Tests

majit-translate lib suite green (3193 passed). One new test pins both directions of the retarget predicate: a raise block holding only a ConstInt is retargeted and then pruned; one holding a getslice — outside CanRemove — keeps its edge and survives the pass.

Note on main's current CI

pyre/check.py is red on ubuntu and windows and the CPython gate is red, all of which predate #1206: main at b0f34c0af3e failed the same three jobs with byte-identical rows — wasm synth/exception_traceback_loop_forms guard_failures 810 -> 811 and cranelift synth/str_fstring 659 -> 658, cranelift 1 failed/424 passed, wasm 1 failed/417 passed. b0f34c0af3e re-recorded exception_traceback_loop_forms for dynasm and cranelift (811) and left wasm at 810. Not touched here.

authored by Claude

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 65d73b2a-e963-43f9-b72d-c8bc13a0a25d

📥 Commits

Reviewing files that changed from the base of the PR and between 27ad14a and c17fa7e.

📒 Files selected for processing (2)
  • majit/majit-translate/src/front/mir.rs
  • majit/majit-translate/src/model.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3c58f596-b329-4aa6-b09c-1bec19d8b9d5

📥 Commits

Reviewing files that changed from the base of the PR and between 3233b3c and 27ad14a.

📒 Files selected for processing (6)
  • majit/majit-ir/src/descr.rs
  • majit/majit-translate/src/codewriter/call.rs
  • majit/majit-translate/src/lib.rs
  • majit/majit-translate/src/model.rs
  • pyre/pyre-jit-trace/build.rs
  • pyre/pyre-jit-trace/src/jitcode_runtime.rs

Walkthrough

The pull request consolidates field-mint census counters, adds offset-source classification, updates runtime statistics and determinism reporting, and strengthens assertion-error graph pruning with validation and tests.

Changes

Field-mint census and determinism

Layer / File(s) Summary
Consolidated census storage
majit/majit-ir/src/descr.rs
FIELD_MINT now provides generated counters, reset, snapshot, iteration, addition, and classified offset telemetry. Existing field-mint counters use the shared storage.
Offset-source resolution
majit/majit-translate/src/codewriter/call.rs, majit/majit-translate/src/lib.rs
Offset lookup distinguishes concrete layouts, template layouts, and accumulated fallback. Layout comparisons use direct field-slice equality.
Census reset and reporting
pyre/pyre-jit-trace/build.rs, pyre/pyre-jit-trace/src/jitcode_runtime.rs
Code generation resets the census. Runtime statistics enumerate census fields dynamically. Determinism reporting handles process-stateful outputs through a shared reporter.

Assertion-error graph pruning

Layer / File(s) Summary
Validated assertion-edge pruning
majit/majit-translate/src/model.rs
Assertion-error exits can be removed through safe single-entry raise blocks. Blocks with unremovable operations, multiple predecessors, or graph-wide single exits remain. Tests cover pruning and preservation cases.

Estimated code review effort: 4 (Complex) | ~45 minutes

Mergeability Score: ⚪ Minimal · up to 27ad1

The change narrows assertion-edge removal to intermediate blocks whose operations are removable, preserving branches that perform non-removable work; focused tests and generated-output comparisons remain unchanged, so no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

Suggested reviewers: lifthrasiir

Poem

I hop through counters, neat and bright,
Concrete and template offsets take flight.
Raise blocks yield when safe and true,
Census stars shine in runtime view.
Thump, thump—the fields align! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes retargeting assertion-raise blocks and avoiding an overly broad removal pass, which matches the PR's main objective.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch residual
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch residual

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27ad14a2cf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread majit/majit-translate/src/model.rs Outdated
Comment on lines +2478 to +2480
let indirect = exit.target != exceptblock;
let assertion_exit = if indirect {
let target = graph.block(exit.target);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore the direct-only assertion predicate

This still broadens simplify.remove_assertion_errors by accepting an edge whose target is an intermediate block rather than graph.exceptblock; checking that the block's operations satisfy CanRemove makes deletion safer, but it does not reproduce upstream's control-flow shape or direct-only condition. Collapse or retarget the raise block in the corresponding simplification phase, then keep this pass's predicate literal, rather than retaining a second implementation with different eligibility rules.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 53df3623ac1, taking the second half of the suggestion literally.

remove_assertion_errors is back to upstream's exit.target is graph.exceptblock with no indirection. The retarget now happens in the simplification sequence that runs before it: retarget_assert_raise_blocks points an edge whose target exists only to raise the implicit AssertionError straight at exceptblock, gated on the block having a single exit carrying that raise and every operation satisfying can_remove_op (CanRemove, simplify.py:411-423). The bypassed block keeps its exits and falls out as unreachable, as eliminate_empty_blocks leaves the blocks it rewires past.

I first tried the phase that already exists for this — collapse_panic_message_chains, whose doc says it collapses panic chains "to the bare raise so remove_assertion_errors prunes the branch as it does for a direct implicit raise" — by widening its panic_block_is_pure_message to the same predicate. That does not work, and the reason is ordering: the phase runs after simplify_lowered_graph, so remove_assertion_errors has already had its chance. Instrumenting the rejection path confirmed neither do_warn_explicit nor w_member_get_direct_kind ever presents an assert-raising block to it; the 24 blocks it does reject across the corpus are all genuine side-effecting calls (handle_alloc_error, os::exit, core::intrinsics::unreachable).

Generated code is unchanged against the previous revision at the same base and corpus: identical jitcodes_index.bin, identical jitcode name list, 2645 jitcodes both, zero jitcodes differing in bytecode, register counts, constant-pool sizes, result types or start points, and the same single unrepairable constants_r entry.

commented by Claude

@youknowone youknowone changed the title majit: gate the assertion-raise indirection on CanRemove majit: retarget bare assertion-raise blocks instead of widening the pass Aug 14, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 53df3623ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +2452 to +2453
let mut raise_args: std::collections::HashMap<BlockId, Vec<LinkArg>> =
std::collections::HashMap::new();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Replace the unbacked HashMap with dense block storage

This new side table is keyed by BlockId, whose dense index already addresses graph.blocks, but there is no corresponding map in upstream simplify.remove_assertion_errors and no justification for departing from its storage shape. Use indexed Vec/Option storage or structure the retargeting without a side table; the repository specifically requires proving upstream semantics before introducing a HashMap and directs dense identity-keyed lookups toward a vector-like container.

AGENTS.md reference: AGENTS.md:L115-L124

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit c17fa7e).
Updated: 2026-08-14T08:10:15.054Z

Files in the reviewed diff
majit/majit-translate/src/front/mir.rs
majit/majit-translate/src/model.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

  • majit/majit-translate/src/model.rs:2445 ↔ rpython/flowspace/flowcontext.py:1280 and rpython/translator/simplify.py:333 — Charon MIR represents an abort/implicit raise as a separate terminating block, whereas PyPy closes the active flow block directly to exceptblock. retarget_assert_raise_blocks restores the upstream direct-edge shape only for constant-AssertionError, single-exit blocks containing solely CanRemove operations; the subsequent remove_assertion_errors predicate is now the literal upstream predicate. This is a source-IR/Rust-MIR structural adaptation, not a parity mismatch.

  • majit/majit-translate/src/model.rs:2605 ↔ rpython/translator/simplify.py:345 — retained CPython-compiler opcode adaptation: after removing an assertion arm, pyre normalizes a remaining integer discriminant edge to unconditional. PyPy’s flow graphs at this stage use Bool/exception switch shapes; Rust-generated enum-discriminant switches require clearing the integer exit metadata.

#1206 taught `remove_assertion_errors` to follow a one-block indirection,
which broadened `simplify.remove_assertion_errors` past its literal
`exit.target is graph.exceptblock` and, because the guard looked only at
the intermediate block's exit count, deleted whatever operations that
block held.  The comment justifying the widening cited `join_blocks`,
which folds a target only into an unconditional predecessor
(`simplify.py:283-286`) — not into the branch this exit leaves.

`retarget_assert_raise_blocks` runs before the pass and points an edge
whose target exists only to raise the implicit `AssertionError` straight
at `exceptblock`, which is where the flow space puts it upstream.  A
block qualifies when it has a single exit carrying that raise and every
operation it holds satisfies `can_remove_op` — `CanRemove`
(`simplify.py:411-423`), the predicate the dead-op pass would apply to
those operations once the raise is gone.  The bypassed block keeps its
exits and falls out as unreachable, as `eliminate_empty_blocks` leaves
the blocks it rewires past.  `remove_assertion_errors` goes back to
upstream's literal predicate.

Two other placements were tried and measured against the corpus:
requiring the intermediate block to be empty, and widening the front's
`panic_block_is_pure_message` so `collapse_panic_message_chains` would
normalise these raises.  Both left all three unrepairable `constants_r`
entries and the `jitcodes.bin` / `jit_metadata.json` in-process
determinism failure in place — the second because the shape never
reaches that phase, which runs after `simplify_lowered_graph`.

Generated code is unchanged against #1206's guard at this base and
corpus: identical `jitcodes_index.bin`, identical jitcode name list, 2645
jitcodes both, zero jitcodes differing in bytecode, register counts,
constant-pool sizes, result types or start points, one unrepairable
`constants_r` entry (`_unpackiterable_unknown_length` `0x8`) on both, and
13/13 reproducible outputs identical in-process.

Assisted-by: Claude
@youknowone
youknowone merged commit f3b6067 into main Aug 14, 2026
8 checks passed
@youknowone
youknowone deleted the residual branch August 14, 2026 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant